test: Refactor mock CSI manager #18554
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
and MockCSIManager to support the call counting that csi_hook_test expects instead of separately implementing csimanager interfaces in two separate places: * client/allocrunner/csi_hook_test * client/csi_endpoint_test they can both use the same mocks defined in client/pluginmanager/csimanager/ alongside the actual implementations of them.
to use MockCSIManager like Node_ExpandVolume so we can also test the happy path
tgross
approved these changes
Sep 21, 2023
There was a problem hiding this comment.
LGTM!
My misgivings: my desire to leave the csi_hook tests mostly unchanged has resulted in a sorta franken-mock where ExpandVolume sticks out as different, since it tracks LastExpandVolumeCall, which I wanted in client/csi_endpoint, whereas allocrunner/csi_hook does call counting.
I get where you're going with that worry, but it also suggests that some of the csi_hook tests could benefit from checking the last call arguments themselves, and not just the counts. It's not 100% consistent, but it's obvious where someone would add that feature to the mock down the road if they need it later. I'm 👍 to merge as-is.
gulducat
added a commit
that referenced
this pull request
Sep 22, 2023
and MockCSIManager to support the call counting that csi_hook_test expects instead of implementing csimanager interfaces in two separate places: * client/allocrunner/csi_hook_test * client/csi_endpoint_test they can both use the same mocks defined in client/pluginmanager/csimanager/ alongside the actual implementations of them. also refactor TestCSINode_DetachVolume to use use it like Node_ExpandVolume so we can also test the happy path there
gulducat
added a commit
that referenced
this pull request
Sep 25, 2023
and MockCSIManager to support the call counting that csi_hook_test expects instead of implementing csimanager interfaces in two separate places: * client/allocrunner/csi_hook_test * client/csi_endpoint_test they can both use the same mocks defined in client/pluginmanager/csimanager/ alongside the actual implementations of them. also refactor TestCSINode_DetachVolume to use use it like Node_ExpandVolume so we can also test the happy path there
gulducat
added a commit
that referenced
this pull request
Sep 25, 2023
and MockCSIManager to support the call counting that csi_hook_test expects instead of implementing csimanager interfaces in two separate places: * client/allocrunner/csi_hook_test * client/csi_endpoint_test they can both use the same mocks defined in client/pluginmanager/csimanager/ alongside the actual implementations of them. also refactor TestCSINode_DetachVolume to use use it like Node_ExpandVolume so we can also test the happy path there
gulducat
added a commit
that referenced
this pull request
Sep 25, 2023
and MockCSIManager to support the call counting that csi_hook_test expects instead of implementing csimanager interfaces in two separate places: * client/allocrunner/csi_hook_test * client/csi_endpoint_test they can both use the same mocks defined in client/pluginmanager/csimanager/ alongside the actual implementations of them. also refactor TestCSINode_DetachVolume to use use it like Node_ExpandVolume so we can also test the happy path there
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right up front: I have mixed feelings about this. Let me know if I should just throw it out 😋
Instead of separately implementing csimanager
interfaces in two separate places:
they can both use the same mocks defined in
client/pluginmanager/csimanager/
alongside the actual implementations of them.
My misgivings: my desire to leave the csi_hook tests mostly unchanged has resulted in a sorta franken-mock where ExpandVolume sticks out as different, since it tracks LastExpandVolumeCall, which I wanted in client/csi_endpoint, whereas allocrunner/csi_hook does call counting.
I'm not thrilled with the result... I might prefer proper call recording across the board, which could also cover call counts, but I should probably move on to the next priority that needs attention. If the state of this is distasteful, I'd be willing to just throw it away. 🤷